CIP-0138 Builtin Array #6727

Unisay · 2024-12-02T13:47:49Z

Closes #6717

Built-in Array type itself, implemented by the Data.Vector.Strict
Builtin funtion LengthArray
Builtin funtion ListToArray
Tests

The PR is ready for review but build is not working until we update flake inputs (need vector 0.13.2)

plutus-core/cost-model/data/builtinCostModelA.json

plutus-core/plutus-core.cabal

plutus-core/plutus-core/src/PlutusCore/Crypto/BLS12_381/Pairing.hs

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs

plutus-core/plutus-core/src/PlutusCore/Pretty/PrettyConst.hs

plutus-core/cost-model/create-cost-model/BuiltinMemoryModels.hs

effectfully

Is the lack of Plinth versions of the builtins intentional?

All looks great. Kinda unfortunate that more and more is needed to add a new builtin, maybe we could automate some more. Although seems not too bad still.

plutus-core/cost-model/create-cost-model/BuiltinMemoryModels.hs

plutus-core/cost-model/data/builtinCostModelA.json

plutus-core/plutus-core/src/Data/Vector/Orphans.hs

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs

plutus-core/plutus-core/test/CostModelSafety/Spec.hs

plutus-core/testlib/PlutusCore/Generators/QuickCheck/Builtin.hs

effectfully · 2024-12-06T16:15:13Z

plutus-core/untyped-plutus-core/test/Evaluation/Builtins/Definition.hs

+      let arrayOfInts = mkConstant @(Vector Integer) @DefaultUni () (Vector.fromList [1..10])
+      let term = apply () (tyInst () (builtin () ListToArray) integer) listOfInts
+      typecheckEvaluateCekNoEmit def defaultBuiltinCostModelForTesting term @?=
+          Right (EvaluationSuccess arrayOfInts)


Some edge cases would be nice, like an empty array etc.

You should've probably used

evals :: DefaultUni `HasTermLevel` a => a -> DefaultFun -> [Type TyName DefaultUni ()] -> [Term TyName Name DefaultUni DefaultFun ()] -> TestNested

from the below, we use it for unit tests. Not important though.

Some edge cases would be nice, like an empty array etc.

Those will definitely get tested in the conformance tests later. Maybe it's OK to have some overlap here though.

Yeah, I think it's better to have some overlap here, since it's cheap.

effectfully · 2024-12-06T16:34:45Z

plutus-core/untyped-plutus-core/test/Evaluation/Builtins/Definition.hs

+          term = apply () (tyInst () (builtin () LengthArray) integer) arrayOfInts
+      typecheckEvaluateCekNoEmit def defaultBuiltinCostModelForTesting term @?=
+          Right (EvaluationSuccess expectedLength)
+    , testCase "indexArray" do


We should also have some property tests, but feel free to add them in a follow-up.

plutus-tx/src/PlutusTx/Builtins/HasBuiltin.hs

Unisay · 2024-12-09T09:06:40Z

Is the lack of Plinth versions of the builtins intentional?

Yes.

…ropriate.

kwxm

This looks OK modulo a few minor quibbles. I'd change the costs in Default.Builtins to be unimplementedCostingFunction just to make sure that we don't get any surprises before the costing's done properly though.

plutus-core/plutus-core/src/PlutusCore/Crypto/BLS12_381/Pairing.hs

plutus-core/plutus-core/src/Data/Vector/Orphans.hs

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs

kwxm · 2024-12-28T01:39:39Z

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs

+              DefaultUniArray _argUni -> pure $ Vector.length vec
+              _ -> throwing _StructuralUnliftingError "Expected an array but got something else"
+          {-# INLINE lengthArrayDenotation #-}
+        in makeBuiltinMeaning lengthArrayDenotation (runCostingFunOneArgument . paramLengthArray)


I wouldn't mind it we said that new builtins must go last and move caseList and caseData there.

Yes, I think we should move them too.

kwxm · 2024-12-28T01:41:15Z

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs

            {-# INLINE nullListDenotation #-}
        in makeBuiltinMeaning
            nullListDenotation
            (runCostingFunOneArgument . paramNullList)

+    toBuiltinMeaning _semvar LengthArray =
+      let lengthArrayDenotation :: SomeConstant uni (Vector a) -> BuiltinResult Int


Maybe call this lengthOfArray for consistency with lengthOfByteString.

I don't mind, @effectfully are you ok with it?

Yes, sorry, I think it was me who gave you the bad advice.

kwxm · 2024-12-31T02:42:17Z

plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/ExMemoryUsage.hs

@@ -293,6 +305,7 @@ addConstantRose (CostRose cost1 forest1) (CostRose cost2 forest2) =
 {-# INLINE addConstantRose #-}

 instance ExMemoryUsage a => ExMemoryUsage [a] where


Nothing to do with this PR really, but I wonder if we can/should make the default memory usage for lists the one that returns the length of the list (ie, the one for the ListCostedByLength wrapper). That's a more natural measure of size for many applications (like listToArray) since in general you'll only be copying pointers rather than reallocating all of the contents of the list. I think this got added for serialiseData (because there you really do have to care about the contents) and it just ended up as the default. We'd have to check its uses carefully if we do want to change it though. Also ExMemoryUsage isn't really the best name in this context since it's really just a size measure.

plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/ExMemoryUsage.hs

plutus-core/plutus-core/src/PlutusCore/Parser/Builtin.hs

kwxm · 2024-12-31T02:57:03Z

plutus-core/plutus-core/src/PlutusCore/Pretty/Extra.hs

@@ -62,3 +62,8 @@ instance PrettyDefaultBy config [a] => DefaultPrettyBy config (Set a) where
    defaultPrettyBy config = prettyBy config . Set.toList
 deriving via PrettyCommon (Set a)
    instance PrettyDefaultBy config (Set a) => PrettyBy config (Set a)
+
+instance PrettyDefaultBy config [a] => DefaultPrettyBy config (Vector a) where
+    defaultPrettyBy config = prettyBy config . toList


I wonder if having identical textual representations could ever lead to ambiguity. I suppose in UPLC it'll always have list or array nearby anyway.

kwxm · 2024-12-31T03:09:52Z

plutus-core/untyped-plutus-core/test/Evaluation/Builtins/Definition.hs

+      let arrayOfInts = mkConstant @(Vector Integer) @DefaultUni () (Vector.fromList [1..10])
+      let term = apply () (tyInst () (builtin () ListToArray) integer) listOfInts
+      typecheckEvaluateCekNoEmit def defaultBuiltinCostModelForTesting term @?=
+          Right (EvaluationSuccess arrayOfInts)


Some edge cases would be nice, like an empty array etc.

Those will definitely get tested in the conformance tests later. Maybe it's OK to have some overlap here though.

kwxm · 2024-12-31T03:25:18Z

I wonder if we have to worry about plutus-metatheory at this stage now. Because of the ceritfication stuff, plutus-executables now depends on plutus-metatheory and when you try to build that you get warnings about miissing cases for the new builtins. These used to be totally independent so you could get away with updating the metaheory in a separate PR, but maybe now there'll be CI problems because of the warnings. That'd be annoying.

Later: I think that it's OK. Even in master, plutus-metatheory doesn't know about caseList and caseData but it doesn't appear to cause any problems in CI.

kwxm · 2024-12-31T03:27:08Z

plutus-tx/src/PlutusTx/Builtins/HasBuiltin.hs

@@ -91,6 +92,10 @@ instance HasFromBuiltin a => HasFromBuiltin (BuiltinList a) where
    type FromBuiltin (BuiltinList a) = [FromBuiltin a]
    fromBuiltin (BuiltinList xs) = map fromBuiltin xs

+instance HasToBuiltin a => HasToBuiltin (Vector a) where
+    type ToBuiltin (Vector a) = BuiltinArray (ToBuiltin a)
+    toBuiltin = useToOpaque (BuiltinArray . map toBuiltin)


This is causing a CI failure because BultinArray doesn't exist yet (in this PR). Can we just remove this from the PR?

You're right, it fails atm. My idea was to first merge adjacent PR (related changes in Plinth) into this one first.

…n [a]`

Unisay self-assigned this Dec 2, 2024

Unisay temporarily deployed to github-pages December 2, 2024 13:47 — with GitHub Actions Inactive